fix(review): bound visual screenshot rendering - #3712
Conversation
|
Important 🟪🟪🟪🟪🟪🟪🟪🟪🟪🟪🟪🟪 🔍 Gittensory is reviewing…AI analysis is in progress. This comment will update when the review is complete. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed · 🟪 Reviewing |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3712 +/- ##
=======================================
Coverage 93.33% 93.33%
=======================================
Files 317 317
Lines 32433 32467 +34
Branches 11892 11901 +9
=======================================
+ Hits 30270 30303 +33
Misses 1530 1530
- Partials 633 634 +1
🚀 New features to boost your workflow:
|
862bc38 to
07750c0
Compare
|
Rebased onto current `main` to clear the `codecov/patch` failure (patch coverage was 83.33%, 4 lines missing in `src/review/visual/shot.ts` — the `page.evaluate()` height-computation callback inside `captureBoundedFullPageShot` was never actually invoked by the test's mock, just short-circuited). The rebase conflicted with `test/unit/visual-shot.test.ts` against the since-merged scroll-through GIF capture feature (#3612/#3688), which had already solved the identical problem (a mocked `evaluate()` that needs to really invoke its callback for coverage) with a cleaner technique than my first attempt — it just calls the callback and swallows the expected "no `document` in this Node test env" throw, rather than stubbing a fake DOM object. Verified that resolution already covers the previously-missing lines (confirmed via local coverage), so I dropped my own redundant fix and kept the merged result. 45/45 tests pass, typecheck/actionlint/audit clean. |
| } | ||
|
|
||
| async function captureBoundedFullPageShot(page: ScreenshotPage, viewport: Viewport): Promise<Uint8Array | null> { | ||
| const height = await page.evaluate(() => { |
There was a problem hiding this comment.
Screenshot bounds check runs in attacker-controlled page context and can be bypassed
page.evaluate() reads DOM height properties that a malicious page can spoof to bypass raster limits.
Query page metrics via a browser API the page cannot tamper with instead of page.evaluate().
AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.
<file name="src/review/visual/shot.ts">
<violation number="1" location="src/review/visual/shot.ts:126">
<priority>P2</priority>
<title>Screenshot bounds check runs in attacker-controlled page context and can be bypassed</title>
<evidence>The captureBoundedFullPageShot function uses page.evaluate(() => { ... body.scrollHeight ... }) to measure document height before screenshot rasterization. Because this JavaScript executes inside the potentially attacker-controlled page context, a malicious page can override document.body.scrollHeight, documentElement.scrollHeight, and related getters (for example via Object.defineProperty) to report a small height, pass the MAX_SCREENSHOT_HEIGHT and MAX_SCREENSHOT_PIXELS checks, and still cause unbounded Chromium raster work when page.screenshot({ fullPage: true }) runs.</evidence>
<recommendation>Use a browser/CDP metric or binding API that the page cannot tamper with to obtain the true rendered document size before rasterizing. If only page.evaluate is available, consider adding a post-screenshot dimension validation (e.g., check actual PNG width/height or browser viewport metrics after capture) and reject if they exceed the claimed bounds.</recommendation>
</violation>
</file>
07750c0 to
0f0cea4
Compare
|
Addressed Superagent's P2 finding (`src/review/visual/shot.ts:126`): the pre-capture height check runs `page.evaluate()` inside the target page's own JS context, so a malicious page can override `document.body.scrollHeight`/`offsetHeight` (e.g. via `Object.defineProperty`) to report a small height, pass the bound, and still get rasterized to its real, oversized size. Added a tamper-proof recheck: decode the actual width/height Chromium wrote into the output PNG's own IHDR chunk (a cheap ~24-byte header read, not a re-render) and reject if those real dimensions exceed the same bounds — independent of anything the page's own JS could influence. 2 new tests (spoofed-height rejection + normal-dimensions acceptance), 47/47 pass, typecheck/actionlint clean. |
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
…he page's own report captureBoundedFullPageShot measured document height via page.evaluate(), which runs inside the screenshotted page's own JS realm -- a hostile page can override scrollHeight/offsetHeight getters to under-report its height and bypass the bound entirely. Re-check the actual rendered PNG's IHDR dimensions after capture, which come from Chromium's rasterizer and cannot be spoofed by page script.
0f0cea4 to
ab12cdb
Compare
Motivation
fullPage: true) let attacker-controlled document height drive unbounded Chromium raster work and unbounded R2 storage from the public/gittensory/shot?url=surface.Description
MAX_SCREENSHOT_HEIGHT,MAX_SCREENSHOT_PIXELS,MAX_SCREENSHOT_BYTES, andSCREENSHOT_TIMEOUT_MSpluscaptureBoundedFullPageShot(page, viewport)which measures document height, enforces the caps, times out the raster, and checks returned PNG bytes.page.screenshot({ type: "png", fullPage: true })call with the bounded helper so oversized or slow renders degrade topng: null.expensiveinrouteClassForPathso the endpoint is rate-limited more strictly.test/unit/visual-shot.test.tsand updatetest/unit/auth.test.tsto cover bounded full-page captures, over-height pages, excessive pixel-area, oversized PNG output, a screenshot timeout, and the route classification.Testing
npx vitest run test/unit/visual-shot.test.ts test/unit/auth.test.tsand all targeted unit tests passed (47 tests passed).npm run typecheckandgit diff --check, both succeeded locally.npm run test:ci, but the environment hit unrelated long-running/unit timeouts during unrelated queue/backfill tests so the full suite could not be completed here.npm audit --audit-level=moderate, but the registry audit endpoint returned403in this environment so that check could not be completed here.Codex Task